Skip to content
This repository has been archived by the owner on Feb 5, 2023. It is now read-only.

Major overhaul #40

Closed
wants to merge 29 commits into from
Closed

Major overhaul #40

wants to merge 29 commits into from

Conversation

thomasjsn
Copy link

@thomasjsn thomasjsn commented Feb 21, 2017

  • Own config file
  • Implemented orderBy
  • Console commands for viewing/creating indices and mapping
  • Elasticsearch specific search method
  • Remove wildcards from query_string search
  • Do filter using filter term

@thomasjsn
Copy link
Author

Oh... I've used some PHP 7 specific functions... Too soon to require PHP7?

@thomasjsn
Copy link
Author

This PR solves #36, #38 and #39.

@ErickTamayo
Copy link
Owner

ErickTamayo commented Feb 24, 2017

@thomasjsn sorry for the delay I've been on a personal matter that needs to be addressed. I will review/test it in the weekend

@thomasjsn
Copy link
Author

@ErickTamayo I've made a IsElasticSearchable interface, which I use when iterating models. Want me to add it? Doesn't really require any testing anyways.

@ErickTamayo
Copy link
Owner

@thomasjsn sure you can include it, I'm planning to review this today and think of new features that we can add to it.

@thomasjsn
Copy link
Author

@ErickTamayo Sure, I'll add it this evening.

@huglester
Copy link

This PR is really VERY huge improvement. Currently working with it.
But for example I search for tag:
"hello", some items get high scoring, all of them, but scoring is:
9.98
9.97 and so on...

But what if I want only those with 'hello' keyword - and order by 'created_at' field.

and this high scoring is not of a big help... that's main issue why I would like to 'disable' default sorting.

thanks for ideas

@thomasjsn
Copy link
Author

@huglester What I do is map tags as an array of text or keywords. The I search in that field only: tags:hello. Then the scoring will be the same for all documents that has "hello" in the tags array. And then order by the orderBy attribute.

@huglester
Copy link

Hello @thomasjsn and thanks for letting know.

so my initial example was:

'sort' => [
                    [
                        'created_at' => [
                            'order' => 'desc',
                        ],
                        'id' => [
                            'order' => 'desc',
                        ],
                    ],
                ],
                'query' => [
                    'bool' => [
                        'must' => [
                            'term' => ['uri' => $query],
                        ],
                    ],
                ],

so basicly sort part is solved now?

But I am not sure on how to search the tag now?
I mean trying along these lines:

$articles = \Webas\ProfilesModule\Entities\Profile::elasticSearch('multi_match', 'uri:Some-Url', [
            'fields' => ['uri'],
        ])->get();

But I also need BOOL must stuff

Thank you

@thomasjsn
Copy link
Author

thomasjsn commented Mar 3, 2017

I often just do a query_string search with field:query. That is the default, unless you have modified the config.

Project::search('tags:mytag`)->orderBy->('created_at')->get();

You can not include the field: in a multi_match search. That only works with query_string.

So either query_string with the search phrase tags:something or multi_match with search phrase something and set the fields property, as you have done.

It helps to familiarize oneself a bit with the different search methods that ElasticSearch provides.

ElasticSearch is extremely configurable, so it's a bit tricky to write a search method that will suit all needs. You can quite easily write your own trait to tailor the search method exactly like you want. Like I have done for counting, see #42. The you can define the search, filters, ordering and everything like you want it. You will need to dig a bit into mapping the returned ids to the eloquent model.

@huglester
Copy link

I see @thomasjsn
Thanks again, I think I managed to solve this. There were some indices generated incorrectly which were causing different problems.

Thank you this 42 example. It's good to be able to pass custom queries like so!

@VRuzhentsov
Copy link

VRuzhentsov commented Mar 7, 2017

What can we do, to merge this PR in origin package?

@ErickTamayo
Copy link
Owner

ErickTamayo commented Mar 7, 2017 via email

@huglester
Copy link

if that matters, I am currently playing this @thomasjsn fork - and not met any problems. VERY HAPPY :)

@thomasjsn
Copy link
Author

I've been using it on my production site for about 2 weeks now ☺️

@huglester
Copy link

I am not using in production. but for around a week in development. and very happy user!

Repository owner locked and limited conversation to collaborators Mar 7, 2017
use Laravel\Scout\Builder as ScoutBuilder;
use Laravel\Scout\Searchable;

trait ElasticSearchable
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not totally agree with making a new trait for it. I understand the elasticSearch method to be able to customize the search but as this is a driver it has to be something that must comply with the current usage of Scout using the Searchable trait.

We might need to get around the searchableWithin method, for example a public property in the Model makes more sense here and then in the Engine just make a ternary.

I guess we can have the trait but we need to make it the ElasticsearchEngine to work out of the box with the Searchable trait.

@@ -0,0 +1,93 @@
<?php
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can have the configuration inside scout.php I would like to have every driver config in the same scout config file.

@@ -44,7 +48,7 @@ public function update($models)
$params['body'][] = [
'update' => [
'_id' => $model->getKey(),
'_index' => $this->index,
'_index' => $model->searchableWithin(),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in trait, we need to make able to work "out of the box" with the Scout Searchable trait

Repository owner unlocked this conversation Mar 8, 2017
@thomasjsn
Copy link
Author

Anyone interested, feel free to make the requested changes. For my own need I will be focusing more on Elastic Search specific methods, and less on compliance with searching through Scout.

@huglester
Copy link

I must agree with @thomasjsn

Probably some improvements could be made. But the problem with scout is that it is bound to Algolia. you can control lot of stuff in algolia web interface and so on.

Elasticsearch is different. There are different versions etc, since this is separate package, I think it can give some improvements over original Scout. And if separe Trait is used, I do not see any harm.

Sometimes making tool to 'agnotic' you loose 80% of funtionality and you are left with the 'basics':
like lets search for "dog" in the index and return results.

But who uses elasticsearch, probably has more needs than basic search, and needs more control over search.

thank you

@VRuzhentsov
Copy link

VRuzhentsov commented Mar 9, 2017

@thomasjsn
Can you please open issues tracker in yours fork?
maybe, i just dont wanna flude in main repository

i have couple of ideas to improve, and questions.

at this moment, origin method toSearchableArray() overrides ES mappings in yours mappings() method, while reindexing all. is it ok?
I'm still dealing with the correct configuration of the model.

although i found, that Scout+ES is incompatible with Eloquent\Model::toArray() method, because if we are store model with it relations in ES, method toArray() anyway reloading model relations from DB. I suggest to somehow override toArray method, with excluding already loaded from ES attributes/relations
What do you think?

Discovered, that laravel's method toArray() is not so cool as i expected

@reinierkors
Copy link

@thomasjsn I can't get the search ordering to work, I'm using the following:
$videos = Video::search($query)->orderBy('publishedAt', 'desc')->paginate(5);

Do you know what could cause this?

@ErickTamayo
Copy link
Owner

@reinierkors please open a issue in the fork, this PR is getting bloated with non-related questions about the pr itself.

Repository owner locked and limited conversation to collaborators Mar 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants